-
Notifications
You must be signed in to change notification settings - Fork 140
updated SponsorshipModel UI #181
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughAdds a new accessible, focus-managed SponsorshipModal React component with a 4-tab interface and dismissal handlers; adjusts app entry (main.tsx) to remove StrictMode, add a runtime root-element check, and simplify some import paths. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant App
participant SponsorshipModal
participant DOM
User->>App: triggers open (click)
App->>SponsorshipModal: render(open=true, onClose)
SponsorshipModal->>SponsorshipModal: useEffect(open) -> save activeElement, attach Escape/backdrop handlers
SponsorshipModal->>DOM: focus dialogRef, render overlay + tab buttons + panels
User->>SponsorshipModal: navigate tabs (click / keyboard)
SponsorshipModal->>SponsorshipModal: update activeTab, manage focus
User->>SponsorshipModal: press Escape or click backdrop or click Close
SponsorshipModal->>App: call onClose()
SponsorshipModal->>SponsorshipModal: remove listeners, restore previous focus
SponsorshipModal->>DOM: unmount modal and overlay
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (5)
Frontend/src/components/SponsorshipModal/SponsorshipModal.tsx (3)
19-34: Add defensive check before restoring focus.Line 33 attempts to restore focus to
prevActivewithout verifying that the element still exists in the DOM or is focusable. If the element was removed while the modal was open, callingfocus()could fail silently or throw an error.Apply this diff to add a defensive check:
return () => { window.removeEventListener("keydown", onKey); - prevActive?.focus(); + if (prevActive && document.contains(prevActive)) { + prevActive.focus(); + } };
167-179: Simplify mailto link handling.Opening a
mailto:URL withwindow.open(mailto, "_blank")is unnecessary and may be blocked by popup blockers. Themailto:protocol doesn't open a new page; it launches the user's email client directly.Apply this diff to simplify:
<button onClick={() => { - // example contact behaviour: open mailto (safe) const mailto = `mailto:[email protected]?subject=Interested in ${encodeURIComponent( t.title || "campaign" )}`; - const win = window.open(mailto, "_blank"); - if (!win) window.location.href = mailto; + window.location.href = mailto; }} className="px-4 py-2 bg-indigo-600 text-white rounded-lg shadow" >
18-35: Consider implementing a focus trap.The current focus management focuses the modal container on open but doesn't prevent users from tabbing out to background content. This violates modal accessibility best practices, as focus should remain trapped within the modal.
Consider using a library like
focus-trap-reactor implementing a custom focus trap that cycles focus back to the first focusable element when the user tabs past the last element (and vice versa).Example with focus-trap-react:
import FocusTrap from 'focus-trap-react'; // Wrap modal content: <FocusTrap> <div ref={dialogRef} tabIndex={-1} className="..."> {/* modal content */} </div> </FocusTrap>Frontend/src/main.tsx (2)
4-4: Use English for code comments.The comment contains non-English text (Hindi/Hinglish). For maintainability in an international codebase, all comments should be in English.
Apply this diff:
-import "./index.css"; // agar file nahi hai to hata sakte ho (nahin to leave it) +import "./index.css"; // can be removed if file doesn't exist
44-45: Root element fallback may cause issues.Creating and appending a root element to
document.bodyif the#rootelement doesn't exist could lead to unexpected rendering behavior and makes debugging harder. It's better to fail fast with a clear error if the expected DOM structure is missing.Apply this diff:
-const rootEl = document.getElementById("root") || document.body.appendChild(document.createElement("div")); -createRoot(rootEl as HTMLElement).render(<TestApp />); +const rootEl = document.getElementById("root"); +if (!rootEl) { + throw new Error("Root element not found. Ensure index.html contains <div id='root'></div>"); +} +createRoot(rootEl).render(<TestApp />);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Frontend/src/components/SponsorshipModal/SponsorshipModal.tsx(1 hunks)Frontend/src/main.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Frontend/src/main.tsx (1)
Frontend/src/components/SponsorshipModal/SponsorshipModal.tsx (1)
SponsorshipModal(14-192)
🔇 Additional comments (1)
Frontend/src/main.tsx (1)
1-45: PR objectives don't match the actual changes.The PR objectives (from linked issue #87) mention implementing custom scrollbar styling with a pink-to-purple gradient in
App.cssandindex.css, but these changes are not present in this PR. Instead, the PR introduces a newSponsorshipModalcomponent and replaces the main application entry point.Please verify:
- Are the scrollbar styling changes in different files not included in this review?
- Should this PR be split into separate PRs (one for the modal component, one for scrollbar styling)?
- Is the PR description accurate, or does it need to be updated to reflect the actual scope of changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Frontend/src/components/SponsorshipModal/SponsorshipModal.tsx (1)
123-123: Remove trailing whitespace.Line 123 contains unnecessary whitespace/empty line within the button element.
Apply this diff:
> {tt.label} - </button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Frontend/src/components/SponsorshipModal/SponsorshipModal.tsx(1 hunks)
🔇 Additional comments (1)
Frontend/src/components/SponsorshipModal/SponsorshipModal.tsx (1)
97-126: LGTM! ARIA tab pattern correctly implemented.The tabbed interface now properly implements the ARIA tab pattern with all three improvements from the previous review:
- ✅
role="tablist"is present (Line 100)- ✅ Arrow key navigation (ArrowRight/Left/Home/End) is implemented via
handleTabKeyDown(Line 113)- ✅ The roving tabindex pattern (
tabIndex={tab === tt.id ? 0 : -1}on Line 111) is correct per the ARIA Authoring Practices GuideNote: The previous review's concern about
tabIndex={-1}removing inactive tabs from the tab sequence was based on a misunderstanding. The roving tabindex pattern (where only the active tab hastabIndex={0}and inactive tabs havetabIndex={-1}) is the recommended approach in the ARIA APG for tab widgets. Arrow keys allow navigation between tabs, not the Tab key.
Closes #87 (UI Enhancement for Sponsorship Modal)
📝 Description
This pull request enhances the UI of the SponsorshipModal component to improve clarity, accessibility, and overall user experience.
The update focuses on:
These improvements ensure the modal feels cleaner, more intuitive, and aligned with modern UI patterns.
🔧 Changes Made
role="tab") and ARIA labels.📷 Screenshots (Before & After)
Before
After
🤝 Collaboration
Collaborated with:
@AOSSIE-Orgmaintainers (review pending)✅ Checklist
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.